Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate web component to webkit #116

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

saadnoor
Copy link
Contributor

@saadnoor saadnoor commented Dec 10, 2021

In this PR:

  • Integration of web-components into webkit
  • Avatar Icon as an example implementation

How to test avatar Icon:

  • go widgets/Drawer/components/DrawerFooter/DrawerFooter.bbj
  • in line number 58, set #drawerDataModel!.getAvatarUrl() as ""
  • you will see a cirlcular avatar containing the name of the user.

@saadnoor saadnoor requested a review from StephanWald December 10, 2021 18:53
StephanWald
StephanWald previously approved these changes Feb 17, 2022
@saadnoor saadnoor force-pushed the Integrate-web-component-to-webkit- branch from 6d23878 to 0fe6673 Compare March 17, 2022 09:25
@saadnoor saadnoor requested a review from TimonGeisbauer March 17, 2022 09:26
@saadnoor saadnoor requested a review from DominicWrege March 17, 2022 16:27
@StephanWald StephanWald requested review from hyyan and SebastianAdams and removed request for DominicWrege and TimonGeisbauer March 18, 2022 09:28
sg!.setContext(sg!.getAvailableContext())
htmlView! = #Wnd!.addHtmlView(wnd!.getAvailableControlID(),0,0,300,300,"<avatar-initial height="""+height$+""" width="""+width$+""" name="""+name$+"""></avatar-initial>")
htmlView! = #configureAvatarView(htmlView!)
methodret htmlView!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Syntax Error: missing methodend

#fireEvent(#ON_AVATAR_INITIAL_CLICK, ev!)
methodend

methodend
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this methodend


methodend

method public BBjHtmlView drawAvatarIcon(BBjWindow wnd!,BBjInt height%, BBjInt width%, BBjString name$)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, it's not duplicate, one function takes integer and other take string (width and height)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need two methods performing the exact same task and executing basically the same thing. The method with the int parameters converts the ints into strings and executes the exact same code as the method with the string parameters, making it duplicate code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah you're right. Got it. Fixing now. Thanks.

methodend

method public BBjHtmlView drawAvatarIcon(BBjWindow wnd!,BBjString height$, BBjString width$, BBjString name$)
htmlView! = wnd!.addHtmlView(wnd!.getAvailableControlID(),0,0,300,300,"<avatar-initial height="""+height$+""" width="""+width$+""" name="""+name$+"""></avatar-initial>")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate code

sg! = BBjAPI().openSysGui("X0")
#Wnd! =sg!.addWindow(95501,0,0,0,0,"",$01101083$)
sg!.setContext(sg!.getAvailableContext())
htmlView! = #Wnd!.addHtmlView(wnd!.getAvailableControlID(),0,0,300,300,"<avatar-initial height="""+height$+""" width="""+width$+""" name="""+name$+"""></avatar-initial>")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate code


method public BBjHtmlView drawAvatarIcon(BBjString height$, BBjString width$, BBjString name$)
sg! = BBjAPI().openSysGui("X0")
#Wnd! =sg!.addWindow(95501,0,0,0,0,"",$01101083$)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id 95501 ?

methodend

method public void onClickAvatarInitial(BBjNativeJavaScriptEvent ev!)
ClientUtil.consoleLog("catched the webcomponent event from BBj :)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this log

@SebastianAdams
Copy link

@saadnoor
Can you please include the javascript code in this pull request so that we can also review that part

@saadnoor
Copy link
Contributor Author

@saadnoor
Can you please include the javascript code in this pull request so that we can also review that part that's already in master, in a seperate repo https://github.com/BasisHub/web-components

@hyyan
Copy link
Member

hyyan commented Mar 18, 2022

@saadnoor I've opened several issues for your avatar component in: https://github.com/BasisHub/web-components/issues

method public AvatarInital(BBjWindow wnd!)
#super!(wnd!)

#setTitle("Avatar Initail Demo")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: It should probably say "Avatar Initial Demo"

use ::WebKit/widgets/WebComponents/WebComponents.bbj::WebComponents
use ::WebKit/widgets/WebComponents/AvatarIcon/AvatarIcon.bbj::AvatarIcon

class public AvatarInital extends ShowcaseWidget

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in class name: It should probably say "AvatarInitial" or "AvatarInitials"

@@ -0,0 +1,71 @@
use ::WebKit/demo/Showcase/ShowcaseWidget/ShowcaseWidget.bbj::ShowcaseWidget

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in file name

Copy link

@SebastianAdams SebastianAdams Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code indentation in the whole file is bad

#super!(wnd!)

#setTitle("Avatar Initail Demo")
#setIntro("This demo shows the Avatar initail stuff")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo + We should refrain from writing "stuff" as this seems unprofessional

@@ -18,6 +19,26 @@ class public CircularAvatar extends BBjWidget
#setCanvas(#window!)
#redraw(1)
methodend

method public CircularAvatar(BBjWindow parent!, BBjString radius!, BBjString imagePath!, BBjString title!)
DynamicLoader.addLocalCSS("WebKit/widgets/common/CircularAvatar/CircularAvatar.css")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DynamicLoader is deprecated, use the ClientUtil instead

use ::WebKit/widgets/WebComponents/WebComponents.bbj::WebComponents
use ::WebKit/util/ClientUtil.bbj::ClientUtil

class public AvatarIcon extends WebComponents

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class should follow the BBjWidget pattern and not use some custom methods to use the widget (drawAvatarIcon)

use ::WebKit/widgets/WebComponents/WebComponents.bbj::WebComponents
use ::WebKit/util/ClientUtil.bbj::ClientUtil

class public AvatarIcon extends WebComponents

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is missing some setters to update the avatar values - See BBjWidget pattern

methodret htmlView!
methodend

method public BBjHtmlView drawAvatarIcon(BBjString height$, BBjString width$, BBjString name$)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class should be a widget meaning that the BBjHtmlView should not be returned. The class might use the BBjHtmlView internally but for external usage it should always be treated as BBjWidget

methodend

method public BBjHtmlView drawAvatarIcon(BBjWindow wnd!,BBjString height$, BBjString width$, BBjString name$)
htmlView! = wnd!.addHtmlView(wnd!.getAvailableControlID(),0,0,300,300,"<avatar-initial height="""+height$+""" width="""+width$+""" name="""+name$+"""></avatar-initial>")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The properties should not be passed using inline css

@SebastianAdams
Copy link

I was only able to review the code because the showcase didn't work on my machine for unknown reasons

@saadnoor
Copy link
Contributor Author

@saadnoor I've opened several issues for your avatar component in: https://github.com/BasisHub/web-components/issues

@hyyan thanks for opening the issues, will address them one by one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants